Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OptInFeatures.md #1006

Merged
merged 7 commits into from
Jun 6, 2022
Merged

Add OptInFeatures.md #1006

merged 7 commits into from
Jun 6, 2022

Conversation

martinbonnin
Copy link
Contributor

@martinbonnin martinbonnin commented Jun 3, 2022

Rendered proposal

This is a work in progress centralising the latest state of graphql/graphql-spec#943 in a more reader-friendly way than the Github thread.

Comments are welcome here or at graphql/graphql-spec#943

}
```

Tools can get a list of `@optIn` features required to use a field (or input field, argument, enum value) using `requiresOptIn`:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do tools also need a mechanism for listing all of the @optIn features in a schema? I'm imagining tools like GraphiQL showing users a list of possible features they can opt into rather than requiring users to somehow obtain this information via documentation or some other non-introspection based mechanism.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point 👍 If types are not included in introspection in the first place, there's no way of knowing what possible values are for includeOptIn...

That'd be an argument for marker directives because these user-defined directive would be returned no matter what.

The other way to look at this would be to return all the fields all the time (no more includeOptIn) and let the tooling (GraphiQL, Playground, etc...) use the logic they find appropriate to show them in autocomplete (or hide them)? I feel like that'd be my favorite solution. I never really understood why some tools would set includeDeprecated: false for an example. Is there a place where there is more context about this?

on FIELD_DEFINITION
| ARGUMENT_DEFINITION
| INPUT_FIELD_DEFINITION
| ENUM_VALUE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could @optIn also apply to type definitions since you might want to expose entirely new types in an experimental feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly? I copy/pasted this list from @deprecated because I started with @experimental being the counterpoint of @deprecated but as we're moving away from this I think it makes a lot of sense.

I guess it has implications in terms of validation though. If a type is @optIn("foo"), should all the usages be optIn("foo") too? That can make a lot of directives?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about type definitions.

You can query over a union or interface, which could mean you get one of many types. Specifying what a server would have to do if you hadn't opt-ed in based on the runtime value makes this much more complex.
I think having a decidable result based only on schema & operation (and regardless of the value) is desirable.

For the same reason, I'm also not sure about ENUM_VALUE

ARGUMENT_DEFINITION and INPUT_FIELD_DEFINITION make sense from a static validation perspective, but I'm unsure if there's real-world need for them.

At the field level it's often things like "this it's hitting a t1.nano and not ready for 1 million users". Experimental input fields and arguments are more likely about business logic and validation correctness. If they are truely doing something newly experimental they could create a new field instead. I'd love to see some examples of situations where this would be a useful addition


Cons:
* more complex
* requires a grammar change

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble understanding this mechanism. Here's what I gather:

The spec defines a directive named @optIn (and in doing so introduces the need to be able to apply directives to directives)

directive @optIn on DIRECTIVE_DEFINITION

Services create a directive for each distinct opt-in feature they want in their schema

directive @experimentalDeploymentApi on FIELD_DEFINITION @optin

type Query {
 deployment: Deployment @experimentalDeploymentApi
}


enum WorkspaceKind {
 CROSS_PROJECT
 CROSS_COMPANY
}
directive @workspaces(kind: WorkspaceKind) on FIELD_DEFINITION @optin

type Deployment {
 workspaces: [Workspace] @workspaces(kind: CROSS_COMPANY)
}

Is my understanding correct?

  • Do clients do something to consume this? Or is it just documenting?

  • It's not clear what extra information would be useful to capture. Some examples might help clarify

  • This appears to hinder the ability to do out-of-band feature opt-ins (HTTP headers) because you don't have an identifier to use. E.g.

headers: {
  "X-ExperimentalApi": "experimentalDeploymentApi"
}

Copy link
Contributor Author

@martinbonnin martinbonnin Jun 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is my understanding correct?

100% correct. I took the liberty of reusing your wording as it's much clearer 👌 and pushed it to the proposal, I hope you don't mind?

Do clients do something to consume this? Or is it just documenting?

Simply documenting. See also my other comment.

It's not clear what extra information would be useful to capture. Some examples might help clarify

To me, the main advantage is that tools can get the list of opt-in features by scanning the directive definitions. They don't have to scan each field and infer unique feature names. So it's a bit easier on the tooling side.

As a side effect, it also allows users to add arguments to their directives (your WorkspaceKind above) although at this point, I'm not sure at all there are use cases for this.

This appears to hinder the ability to do out-of-band feature opt-ins (HTTP headers) because you don't have an identifier to use. E.g.

You can always use directive name. Your example would actually work

headers: {
  "X-ExperimentalApi": "experimentalDeploymentApi"
}


In other words, `@optIn` arguments or input fields, must be either nullable or have a default value.

## 🗳️ Alternate solutions
Copy link

@tomgasson tomgasson Jun 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative proposal: @requiresOptIn and @OptIn

Similar to the Proposed solution, except the schema uses @requiresOptIn instead

directive @requiresOptIn(feature: String!) on FIELD_DEFINITION 
type Session {
  id: ID!
  title: String!
  startInstant: Instant @requiresOptIn(feature: "experimentalInstantApi")
  endInstant: Instant @requiresOptIn(feature: "experimentalInstantApi")
}

Operations use @optIn

directive @optIn(feature: String!) on FIELD 
query {
  session {
    id
    title
    startInstant @optIn(feature: "experimentalInstantApi")
    endInstant @optIn(feature: "experimentalInstantApi")
  }
}

Adding @requiresOptIn to an existing field would need to be detected as a schema breaking change.
Removing @requiresOptIn is safe, and an operation using @optIn on a field without @requiresOptIn would continue to work.
Tools can use introspection values to provide code actions to add @optIn where necessary
Matching the feature prevents misuse, for example:

query {
  session @optIn {
    id @optIn
    title @optIn
    startInstant @optIn
    endInstant @optIn
  }
}

For introspection, unlike the Proposed solution the schema is telling you about the opt-in features, rather than you having to provide them

 type __Type {
   # [...] other fields omitted for clarity
   fields(
     includeDeprecated: Boolean = false,
+    includeRequiresOptIn: Boolean = false
   ): [__Field!]
 }
 
 type __Field {
   name: String!
   # [...] other fields omitted for clarity
   isDeprecated: Boolean!
   deprecationReason: String
+  requiresOptIn: Boolean!
+  optInFeature: String
 }

Pros:

  • Self-documenting. There's no set of feature names defined somewhere else
  • Usage is colocated with consumption
  • No grammar changes
  • No need to enumerate all opt in features

Cons:

  • Only uses a string, no capability for providing more type information
  • Goes further than just being documentation to somewhat imply behaviour (although that could still be for server to decide)
  • No place to document a feature itself (i.e one feature might apply to many fields, and want a high-level description of the feature itself)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative proposal: @requiresOptIn

Woops, sorry I missed this earlier today, I actually just pushed @requiresOptIn, I like this a lot, it plays well with @optIn and is more explicit

Operations use @optIn

Do we need to put that in the spec? This has execution implication and different teams might want to implement that differently.

I initially envisioned the proposal as a way to "communicate" opt-in requirements (like @deprecated communicates the deprecated status). "Enforcing" it seems another steps with a wide range of implications.

For introspection, unlike the Proposed solution the schema is telling you about the opt-in features, rather than you having to provide them

Not sure I get this one. The current proposed solution already exposes the opt-in features:

type __Field {
  name: String!
  isDeprecated: Boolean!

  # [...] 

  # list of @requiresOptIn features required to use this field
  requiresOptIn: [String!]

We can use an additional Boolean but it feels like duplication because we can tell if there's the need for duplication if the list is not empty.

On a separate (but related) note, I wish isDeprecated would be replaced by checking for deprecationReason != null.

Indicates that the given field, argument, input field or enum value requires
giving explicit consent before being used.
"""
directive @optIn(feature: String!) repeatable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be be repeatable? I'm not sure if that's necessary and it could complicate things

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. Since there are different features I figured out it was possible to require multiple of them and it didn't feel too complex but we can start simple and add repeatable down the road if we want to I guess. Although that circles back to the introspection discussion: if we add repeatable, then we need to model features as a list which might be breaking, etc...

on FIELD_DEFINITION
| ARGUMENT_DEFINITION
| INPUT_FIELD_DEFINITION
| ENUM_VALUE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about type definitions.

You can query over a union or interface, which could mean you get one of many types. Specifying what a server would have to do if you hadn't opt-ed in based on the runtime value makes this much more complex.
I think having a decidable result based only on schema & operation (and regardless of the value) is desirable.

For the same reason, I'm also not sure about ENUM_VALUE

ARGUMENT_DEFINITION and INPUT_FIELD_DEFINITION make sense from a static validation perspective, but I'm unsure if there's real-world need for them.

At the field level it's often things like "this it's hitting a t1.nano and not ready for 1 million users". Experimental input fields and arguments are more likely about business logic and validation correctness. If they are truely doing something newly experimental they could create a new field instead. I'd love to see some examples of situations where this would be a useful addition

@benjie
Copy link
Member

benjie commented Jun 5, 2022

@martinbonnin Ping me when you want me to merge this. In general RFC documents get merged quickly so that others can raise PRs against them. Also please add yourself as the champion near the top of the document 👍

@martinbonnin
Copy link
Contributor Author

martinbonnin commented Jun 5, 2022

so that others can raise PRs against them

Ah, that makes a lot of sense 👍 . Feel free to merge for me.

@tomgasson I already included some of your feedbacks there (Atlassian prior work and @requiresOptIn). Let me know if you want me to revert these commits so that you can open a PR and get proper attribution.

@benjie
Copy link
Member

benjie commented Jun 6, 2022

Since @tomgasson thumbs-up'd your comment I'm taking that as a signal to merge 👍

@benjie benjie merged commit cc4f578 into graphql:main Jun 6, 2022
@LouisCAD
Copy link

LouisCAD commented Jun 7, 2022

For the case of Kotlin, I think you mentioned @OptIn in place of @RequiresOptIn, which is the replacement for @Experimental.

@martinbonnin
Copy link
Contributor Author

@LouisCAD good catch! #1013 fixes it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants